-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[docs] Able to load doc components inside markdown files #34243
[docs] Able to load doc components inside markdown files #34243
Conversation
163faa6
to
63ac0b8
Compare
@flaviendelangle If my understanding is correct, you want to reuse the component with default props right? The benefit of the new approach is to reduce the demo files, are there other benefits to this? My comment about the usage: // From
{{"component": "modules/components/SelectorsDocs.js", "category": "Visible Columns"}}
// To
{{"component": "modules/components/SelectorsDocs.js", "props": { "category": "Visible Columns" }}} I rather group the properties under "props" to avoid props overriding. |
Concerning the way we pass props, I kept the synthax used for
But if we want to change the signature for something more explicit, that's fine for me.
I have two main reasons
Something like
|
@oliviertassinari @siriwatknp if you can take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks fair enough
|
||
export default function Page(props) { | ||
const { versions } = props; | ||
return ( | ||
<VersionsContext.Provider value={versions}> | ||
<MarkdownDocs demos={demos} docs={docs} demoComponents={demoComponents} /> | ||
<MarkdownDocs {...pageProps} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better with?
<MarkdownDocs {...pageProps} /> | |
<MarkdownDocs pageProps={pageProps} /> |
My concern is about how can we help the future developers to find where these props are coming from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main problem is that it was a major pain to add a new prop on all those files because it was not spread.
And it's very easy to forget one file and have missing content that will be hard to spot.
And I don't see how the current version solves this: help the future developers to find where these props are coming from
If I want to know what is passed to MarkdownDocs
, I can check the props it receives.
The hard part when I tried to understand how everything works was to find the loader.js
file, which took me some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @flaviendelangle, the MarkdownDocs
is the only component that connects to the {{ }}
format anyway. I don't see why we need to pass props as pageProps
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only value would be about making how props drill down a bit more explicit. Spreading tends to hide the actual structure of the props that the component receives. If this argument doesn't resonate, then please ignore my initial comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I misread your comment, I thought you wanted to keep today's signature.
No strong opinion between your proposal and mine then.
…-component-md-loader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
Follow up on #26774
Fixes this TODO:
material-ui/docs/src/modules/components/MarkdownDocs.js
Line 14 in adda22c
I want to use the
{{ "component" }}
synthax on MUI-X to inject some new components.But I can't because the light of available components is hardcoded in this repository.
srcComponents
(better name) export that contains the custom components for the current pageMarkdownDocs
render the custom components usingsrcComponents
SelectorsDocs
mui-x#6103)https://deploy-preview-34243--material-ui.netlify.app/material-ui/react-button/